benchmark: add ipc support to benchmark spawn stdio config by thisalihassan · Pull Request #52456 · nodejs/node

thisalihassan · 2024-04-10T19:46:18Z

Fix (ipc) in the stdio configuration of the spawn function within the benchmark subsystem.

This PRs fixes the mistake in PR (benchmark: conditionally use spawn with taskset for CPU pinning) where I removed IPC by mistake due to which child.on("message") listener wasn't working.

I also made sure of cpu usage of cores provided to taskset and checked if the csv is all good

Fixes: #52233
Refs: nodejs/performance#161

Enabled inter-process communication (ipc) in the stdio configuration of the spawn function within the benchmark subsystem. This change allows for improved data exchange between parent and benchmarked child processes, addressing limitations in performance testing scenarios. Fixes: nodejs#52233 Refs: nodejs/performance#161

rluvaton · 2024-04-10T19:52:33Z

Please remove the pipe to process stdout as this will mess up the compare/run output which should be csv

thisalihassan · 2024-04-10T19:58:49Z

Please remove the pipe to process stdout as this will mess up the compare/run output which should be csv

okay note that it will not print console.logs/error of child scripts in the terminal then while with fork pipes are established automatically

rluvaton · 2024-04-10T20:03:33Z

That was the old behavior, no?

thisalihassan · 2024-04-10T20:12:54Z

That was the old behavior, no?

when not using CPUSET command benchmark will use fork to run the child scripts and fork prints out console.log/console.errors..info..warn of forked instance to the parent instance thus you can see the logs in the terminal.

I wanted to maintain that behavior of fork while using spawn, so that's why we ned to pipe stdio and stderr to the parent (Fork does this by default)

targos · 2024-04-10T20:27:05Z

I wanted to maintain that behavior of fork while using spawn, so that's why we ned to pipe stdio and stderr to the parent (Fork does this by default)

I know it's unrelated to this PR, but wouldn't 'inherit' instead of 'pipe' in the second and third positions make it simpler (then you could remove the manual piping to the parent)

mcollina

lgtm

rluvaton

LGTM

nodejs-github-bot · 2024-04-12T09:58:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58310/

nodejs-github-bot · 2024-04-12T13:25:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58322/

nodejs-github-bot · 2024-04-12T19:51:56Z

Commit Queue failed

- Loading data for nodejs/node/pull/52456
✔  Done loading data for nodejs/node/pull/52456
----------------------------------- PR info ------------------------------------
Title      benchmark: add ipc support to benchmark spawn stdio config (#52456)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     thisalihassan:benchmark-fix-spawn-ipc -> nodejs:main
Labels     benchmark, performance, author ready, commit-queue-rebase
Commits    2
 - benchmark: add ipc support to spawn stdio config
 - benchmark: inherit stdio/stderr instead of pipe
Committers 1
 - Ali Hassan 
PR-URL: https://github.com/nodejs/node/pull/52456
Fixes: https://github.com/nodejs/node/issues/52233
Refs: https://github.com/nodejs/performance/pull/161
Reviewed-By: Matteo Collina 
Reviewed-By: Raz Luvaton 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52456
Fixes: https://github.com/nodejs/node/issues/52233
Refs: https://github.com/nodejs/performance/pull/161
Reviewed-By: Matteo Collina 
Reviewed-By: Raz Luvaton 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 10 Apr 2024 19:46:18 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/52456#pullrequestreview-1993644517
   ✔  - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/52456#pullrequestreview-1994683235
   ✘  Last GitHub CI failed
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8667228479

thisalihassan · 2024-04-12T19:53:51Z

@rluvaton windows tests are failing for some reason

nodejs-github-bot · 2024-04-13T06:32:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58337/

nodejs-github-bot · 2024-04-13T17:41:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58347/

nodejs-github-bot · 2024-04-13T18:01:30Z

Landed in 3634f9c...82891ae

Enabled inter-process communication (ipc) in the stdio configuration of the spawn function within the benchmark subsystem. This change allows for improved data exchange between parent and benchmarked child processes, addressing limitations in performance testing scenarios. Fixes: #52233 Refs: nodejs/performance#161 PR-URL: #52456 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

Enabled inter-process communication (ipc) in the stdio configuration of the spawn function within the benchmark subsystem. This change allows for improved data exchange between parent and benchmarked child processes, addressing limitations in performance testing scenarios. Fixes: #52233 Refs: nodejs/performance#161 PR-URL: #52456 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

Enabled inter-process communication (ipc) in the stdio configuration of the spawn function within the benchmark subsystem. This change allows for improved data exchange between parent and benchmarked child processes, addressing limitations in performance testing scenarios. Fixes: #52233 Refs: nodejs/performance#161 PR-URL: #52456 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

Enabled inter-process communication (ipc) in the stdio configuration of the spawn function within the benchmark subsystem. This change allows for improved data exchange between parent and benchmarked child processes, addressing limitations in performance testing scenarios. Fixes: #52233 Refs: nodejs/performance#161 PR-URL: #52456 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. performance Issues and PRs related to the performance of Node.js. labels Apr 10, 2024

thisalihassan mentioned this pull request Apr 10, 2024

Hetzner Benchmarking Machine Replacements nodejs/build#3657

Open

rluvaton mentioned this pull request Apr 10, 2024

run compare on perf cores nodejs/performance#162

Merged

benchmark: inherit stdio/stderr instead of pipe

001eb87

mcollina approved these changes Apr 11, 2024

View reviewed changes

rluvaton approved these changes Apr 11, 2024

View reviewed changes

rluvaton added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2024

rluvaton mentioned this pull request Apr 11, 2024

Use again the cpuset after compare function fix nodejs/performance#163

Merged

rluvaton added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 11, 2024

mcollina added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 12, 2024

rluvaton added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 13, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2024

nodejs-github-bot closed this Apr 13, 2024

rluvaton mentioned this pull request Apr 13, 2024

test benchmark change #52513

Closed

aduh95 pushed a commit that referenced this pull request Apr 29, 2024

benchmark: inherit stdio/stderr instead of pipe

7cdfe8a

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

aduh95 mentioned this pull request Apr 30, 2024

2024-05-02, Version 22.1.0 (Current) #52768

Merged

marco-ippolito pushed a commit that referenced this pull request May 2, 2024

benchmark: inherit stdio/stderr instead of pipe

c96df78

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

marco-ippolito mentioned this pull request May 2, 2024

v20.13.0 proposal #52793

Merged

marco-ippolito pushed a commit that referenced this pull request May 3, 2024

benchmark: inherit stdio/stderr instead of pipe

2274d0c

PR-URL: #52456 Fixes: #52233 Refs: nodejs/performance#161 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Raz Luvaton <rluvaton@gmail.com>

Uh oh!

Conversation

thisalihassan commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rluvaton commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisalihassan commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rluvaton commented Apr 10, 2024

Uh oh!

thisalihassan commented Apr 10, 2024

Uh oh!

targos commented Apr 10, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 12, 2024

Uh oh!

thisalihassan commented Apr 12, 2024

Uh oh!

nodejs-github-bot commented Apr 13, 2024

Uh oh!

nodejs-github-bot commented Apr 13, 2024

Uh oh!

nodejs-github-bot commented Apr 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thisalihassan commented Apr 10, 2024 •

edited

Loading

rluvaton commented Apr 10, 2024 •

edited

Loading

thisalihassan commented Apr 10, 2024 •

edited

Loading